Fix crash rewriting relative document URI on ..// in querystring#314
Fix crash rewriting relative document URI on ..// in querystring#314goosfrabba wants to merge 1 commit into
..// in querystring#314Conversation
benoit74
left a comment
benoit74
left a comment
There was a problem hiding this comment.
Thank you for the valuable PR.
Two things to fix to move PR forward:
- you've fixed URL rewriting in Python which is good ; we need to have the same logic and fixes applied to URL rewriting in Javascript (see )
- your comments currently explain why you've done these changes ; they should be meant to help future maintainer which barely know about this PR ; for instance "it used to raide
ValueError..." is quite verbose and I don't feel like it help future maintainer, which probably does not care what the code used to misbehave at some point in the past.
benoit74
left a comment
benoit74
left a comment
There was a problem hiding this comment.
Reading this twice, the fix is significantly wrong.
If current page is http://kiwix.org/common/sub/page.html?css=../../prg, then its URL will in fact be http://kiwix.org/common/sub/page.html%3Fcss=../../prg to remove the query-string so that it does not causes issues with real query parameters in some URLs in some readers.
The problem is that while this convention allows to replace query-strings by paths, we end-up with a /../ in this item path, which will be interpreted legitimately as a relative path instruction and is the root-cause of the issue at hand.
Not transforming the query-string is wrong because it will cause issues in many readers.
Ignoring path segments in query-string is wrong because they are not query-strings anymore in the ZIM, but path segments.
@goosfrabba do you intend to work on this soon or should I take over?
f08bb5a to
e1f0cb5
Compare
|
I just rebased the branch to accomodate other changes which occured in parallel |
`ArticleUrlRewriter.get_document_uri` computed the relative path between two ZIM entries by feeding `path + "?" + querystring` to `PurePosixPath`, which splits on `/` and interprets `..` segments as directory navigation. When the document being rewritten (or a linked item) had a querystring containing `..` (e.g. `xtree.html?css=../../prg`), `PurePosixPath.relative_to(..., walk_up=True)` raised `ValueError: '..' segment ... cannot be walked`, aborting the scrape. A querystring is part of the ZIM entry name (a leaf), not a navigable directory, so its content must not take part in the relative-path walk. Compute the relative path from the path components only, then re-append the querystring (url-encoded together with the path) once the relative path is known. Existing outputs are unchanged for querystrings without `/` or `..` segments. Fixes openzim/warc2zim#380 Co-Authored-By: Claude Opus 4.8 <[email protected]>
e1f0cb5 to
2c2ff2a
Compare
2 participants
What
ArticleUrlRewriter.get_document_uricrashes when the document being rewritten (or a linked item) has a querystring that contains a..segment, e.g.xtree.html?css=../../prg:This is the bug reported (and confirmed as legitimate by @benoit74) in openzim/warc2zim#380 — the rewriting code has since moved into this library, so the fix belongs here.
Why it happened
get_document_uribuilt the stringpath + "?" + querystringand handed it toPurePosixPath.PurePosixPathsplits on/and treats..as directory navigation, so any/or..inside a querystring was interpreted as path structure.PurePosixPath.relative_to(..., walk_up=True)then raisesValueErroras soon as it has to walk up through a..segment coming from the querystring.Fix
A querystring is part of the ZIM entry name (a leaf), not a navigable directory, so its content must not take part in the relative-path walk. The relative path is now computed from the path components only, and the querystring is re-appended (url-encoded together with the path) once the relative path is known.
Output is unchanged for querystrings that don't contain
/or..segments (verified by the existing test suite).Tests
Added
test_get_document_uri_querystring_segmentscovering..in the document querystring (the crash case),..in a linked item's querystring, and/in a querystring.Before the fix (source reverted, test kept):
After the fix:
Full
tests/rewriting/suite passes;ruff==0.15.14check + format clean.Disclosure: implemented with the help of an AI coding assistant (Claude); verified locally with the included test.
Fixes #321